Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make preprocessors mask_above/below_threshold, mask_inside/outside_range lazy #2169

Merged
merged 10 commits into from
Aug 21, 2023
Merged

Make preprocessors mask_above/below_threshold, mask_inside/outside_range lazy #2169

merged 10 commits into from
Aug 21, 2023

Conversation

joergbenke
Copy link
Contributor

@joergbenke joergbenke commented Aug 10, 2023 •
edited by schlunma
Loading

Description

  • addresses parts of Make preprocessor lazy #674
  • the functions mask_above_threshold, mask_below_threshold, mask_inside_range and
    mask_outside_range have been daskified (substituting numpy with dask.array)
  • they were tested against the recipe "recipe_mask_landsea.yml"
  • the reduction of memory consumption was in this case on Levante (login nodes) from approx 20 GiB to 12 GiB, runtime improvement was about 20% (distributed.LocalCluster)
  • I've added my name to the CITATION.cff and .zenode.json files

Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

To help with the number pull requests:

@codecov
Copy link

codecov bot commented Aug 10, 2023 •
edited
Loading

Codecov Report

Merging #2169 (9a83d49) into main (d518743) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2169   +/-   ##
=======================================
  Coverage   93.10%   93.10%           
=======================================
  Files         237      237           
  Lines       12826    12826           
=======================================
  Hits        11942    11942           
  Misses        884      884
Files Changed Coverage Δ
esmvalcore/preprocessor/_mask.py 86.40% <100.00%> (ø)

@valeriupredoi
Copy link
Contributor

hi @joergbenke many thanks for contributing! Could you please tell us what this PR does, by adding a description to it (the title could use with a bit more info too please), and also if there is any issue that this contributes towards or closes; also please make sure the boxes in the PR description are ticked - the ones that are relevant to this PR, otherwise the others that are not, should either be strike-thru-ed, or removed. Then I'll be happy to review it 🍻

@joergbenke joergbenke added enhancement New feature or request dask related to improvements using Dask labels Aug 17, 2023
@joergbenke joergbenke added this to the v2.10.0 milestone Aug 17, 2023
@joergbenke joergbenke self-assigned this Aug 17, 2023
@joergbenke joergbenke changed the title Development dask natesm Make preprocessors mask_above_threshold, mask_below_threshold, mask_inside_range and mask_outside_range lazy Aug 17, 2023
@joergbenke joergbenke changed the title Make preprocessors mask_above_threshold, mask_below_threshold, mask_inside_range and mask_outside_range lazy Make preprocessors mask_above/below_threshold, mask_inside/outside_range lazy Aug 17, 2023
@joergbenke
Copy link
Contributor Author

I've adapted my pull request and did some minor corrections:

  • I've adapted manually the .zenode file,
  • checked the topics of the checklist, etc.,
  • wrote some aditional information (Indeed I posted it already with my last Pull Request but it was still commented out)

I hope everything is fine now.

remi-kazeroni
remi-kazeroni approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution, @joergbenke! The changes look good to me. I also noticed the performance improvements by running test cases with distributed.LocalCluster which could be run successfully with this branch but not with current main. This PR follows our guidelines, it is ready to be merged 👍

@schlunma
Copy link
Contributor

Thanks @joergbenke for your contribution and @remi-kazeroni for reviewing! This can be merged! I also checked the corresponding boxes in #674, 4 more prepcessory lazy 🚀

@schlunma schlunma merged commit 4fd8701 into main Aug 21, 2023
2 checks passed
@schlunma schlunma deleted the development_dask_natesm branch August 21, 2023 15:29
@bouweandela bouweandela mentioned this pull request Nov 1, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remi-kazeroni remi-kazeroni remi-kazeroni approved these changes

@schlunma schlunma Awaiting requested review from schlunma

Assignees

@joergbenke joergbenke

Labels
dask related to improvements using Dask enhancement New feature or request
Projects
None yet
Milestone
v2.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@joergbenke @valeriupredoi @schlunma @remi-kazeroni